Skip to content

fix(firestore): Further improved performance of UTF-8 string comparison logic #7098

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 36 commits into from
Jul 7, 2025

Conversation

dconeybe
Copy link
Contributor

@dconeybe dconeybe commented Jul 3, 2025

Further improved performance of UTF-8 string ordering logic, which had degraded in v25.1.2 due to the fix #6615 and received some improvements in v25.1.3 by the fix #6706. Even with these improvements, customers still observed a material performance degradation (#7053), which this PR addresses.

This PR was ported to the other Firestore SDKs:

Porting to firebase-ios-sdk is not required because it uses UTF-8 encoding natively. In contrast, Java and JavaScript use UTF-16 natively and require special comparison logic to order strings by their UTF-8 encoding lexicographically.

Fixes: #7053

Copy link
Contributor

github-actions bot commented Jul 3, 2025

Test Results

  186 files  +  144    186 suites  +144   4m 41s ⏱️ + 3m 19s
1 235 tests +  910  1 219 ✅ +  894  16 💤 +16  0 ❌ ±0 
2 494 runs  +1 832  2 462 ✅ +1 800  32 💤 +32  0 ❌ ±0 

Results for commit 3c9caad. ± Comparison against base commit aad3da8.

This pull request removes 325 and adds 1235 tests. Note that renamed tests count towards both.
com.google.firebase.remoteconfig.ConfigTests ‑ Custom Signals builder support multiple types
com.google.firebase.remoteconfig.ConfigTests ‑ Firebase#remoteConfig should delegate to FirebaseRemoteConfig#getInstance()
com.google.firebase.remoteconfig.ConfigTests ‑ Firebase#remoteConfig should delegate to FirebaseRemoteConfig#getInstance(FirebaseApp, region)
com.google.firebase.remoteconfig.ConfigTests ‑ FirebaseRemoteConfigSettings builder works
com.google.firebase.remoteconfig.ConfigTests ‑ Overloaded get() operator returns default value when key doesn't exist
com.google.firebase.remoteconfig.ConfigTests ‑ Overloaded get() operator returns value when key exists
com.google.firebase.remoteconfig.CustomSignalsTest ‑ testCustomSignals_builderPutDouble
com.google.firebase.remoteconfig.CustomSignalsTest ‑ testCustomSignals_builderPutDuplicateKeys
com.google.firebase.remoteconfig.CustomSignalsTest ‑ testCustomSignals_builderPutLong
com.google.firebase.remoteconfig.CustomSignalsTest ‑ testCustomSignals_builderPutMixedTypes
…
com.google.firebase.firestore.AggregateQuerySnapshotTest ‑ createWithCountShouldReturnInstanceWithTheGivenQueryAndCount
com.google.firebase.firestore.AggregateQueryTest ‑ testSourceMustNotBeNull
com.google.firebase.firestore.BlobTest ‑ testComparison
com.google.firebase.firestore.BlobTest ‑ testEquals
com.google.firebase.firestore.BlobTest ‑ testMutableBytes
com.google.firebase.firestore.CollectionReferenceTest ‑ testEquals
com.google.firebase.firestore.DocumentChangeTest ‑ randomTests
com.google.firebase.firestore.DocumentChangeTest ‑ testAdditions
com.google.firebase.firestore.DocumentChangeTest ‑ testChangesWithSortOrderChange
com.google.firebase.firestore.DocumentChangeTest ‑ testDeletions
…

♻️ This comment has been updated with latest results.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jul 3, 2025

Size Report 1

Affected Products

  • firebase-firestore

    TypeBase (aad3da8)Merge (f9fdc10)Diff
    aar1.45 MB1.45 MB-193 B (-0.0%)
    apk (release)11.4 MB11.4 MB-84 B (-0.0%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/7vkxXByA5M.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jul 3, 2025

Startup Time Report 1

Note: Layout is sometimes suboptimal due to limited formatting support on GitHub. Please check this report on GCS.

Notes

Startup Times

  • fire-fst

    DeviceStatisticsDistributions
    oriole-32
    Percentileaad3da8f9fdc10DiffSignificant (?)
    p10276 ±12 μs274 ±15 μs-1.62 μs (-0.6%)NO
    p25285 ±15 μs283 ±16 μs-2.64 μs (-0.9%)NO
    p50303 ±24 μs299 ±23 μs-3.52 μs (-1.2%)NO
    p75358 ±79 μs364 ±78 μs+6.24 μs (+1.7%)NO
    p90404 ±111 μs407 ±102 μs+2.63 μs (+0.7%)NO

    20 test runs in comparison
    CommitTest Runs
    aad3da8
    • 2025-07-04_17:50:47.786239_CMwB
    • 2025-07-04_17:50:47.786285_xldx
    • 2025-07-04_17:50:47.786295_Hhdf
    • 2025-07-04_17:50:47.786304_qdyr
    • 2025-07-04_17:50:47.786312_YotG
    • 2025-07-04_17:50:47.786319_bWyP
    • 2025-07-04_17:50:47.786325_ZgLW
    • 2025-07-04_17:50:47.786332_Ravw
    • 2025-07-04_17:50:47.786338_ODeW
    • 2025-07-04_17:50:47.786344_adtW
    f9fdc10
    • 2025-07-07_15:41:50.440045_YTFm
    • 2025-07-07_15:41:50.440081_PHFe
    • 2025-07-07_15:41:50.440093_XrRb
    • 2025-07-07_15:41:50.440104_kHKZ
    • 2025-07-07_15:41:50.440112_oqDE
    • 2025-07-07_15:41:50.440120_AvxA
    • 2025-07-07_15:41:50.440127_aeiG
    • 2025-07-07_15:41:50.440134_Slsj
    • 2025-07-07_15:41:50.440141_IaEq
    • 2025-07-07_15:41:50.440148_WrIl
    redfin-30
    Percentileaad3da8f9fdc10DiffSignificant (?)
    p10498 ±51 μs483 ±30 μs-15.4 μs (-3.1%)NO
    p25512 ±49 μs501 ±35 μs-11.0 μs (-2.1%)NO
    p50531 ±48 μs527 ±40 μs-4.10 μs (-0.8%)NO
    p75557 ±49 μs553 ±47 μs-3.54 μs (-0.6%)NO
    p90593 ±48 μs584 ±52 μs-8.89 μs (-1.5%)NO

    20 test runs in comparison
    CommitTest Runs
    aad3da8
    • 2025-07-04_17:50:47.786239_CMwB
    • 2025-07-04_17:50:47.786285_xldx
    • 2025-07-04_17:50:47.786295_Hhdf
    • 2025-07-04_17:50:47.786304_qdyr
    • 2025-07-04_17:50:47.786312_YotG
    • 2025-07-04_17:50:47.786319_bWyP
    • 2025-07-04_17:50:47.786325_ZgLW
    • 2025-07-04_17:50:47.786332_Ravw
    • 2025-07-04_17:50:47.786338_ODeW
    • 2025-07-04_17:50:47.786344_adtW
    f9fdc10
    • 2025-07-07_15:41:50.440045_YTFm
    • 2025-07-07_15:41:50.440081_PHFe
    • 2025-07-07_15:41:50.440093_XrRb
    • 2025-07-07_15:41:50.440104_kHKZ
    • 2025-07-07_15:41:50.440112_oqDE
    • 2025-07-07_15:41:50.440120_AvxA
    • 2025-07-07_15:41:50.440127_aeiG
    • 2025-07-07_15:41:50.440134_Slsj
    • 2025-07-07_15:41:50.440141_IaEq
    • 2025-07-07_15:41:50.440148_WrIl
  • timeToInitialDisplay

    DeviceStatisticsDistributions
    oriole-32
    Percentileaad3da8f9fdc10DiffSignificant (?)
    p10199 ±3 ms201 ±2 ms+2.77 ms (+1.4%)NO
    p25204 ±4 ms207 ±3 ms+3.58 ms (+1.8%)NO
    p50210 ±4 ms215 ±3 ms+4.95 ms (+2.4%)NO
    p75216 ±4 ms223 ±4 ms+6.72 ms (+3.1%)NO
    p90224 ±5 ms235 ±6 ms+10.1 ms (+4.5%)NO

    20 test runs in comparison
    CommitTest Runs
    aad3da8
    • 2025-07-04_17:50:47.786239_CMwB
    • 2025-07-04_17:50:47.786285_xldx
    • 2025-07-04_17:50:47.786295_Hhdf
    • 2025-07-04_17:50:47.786304_qdyr
    • 2025-07-04_17:50:47.786312_YotG
    • 2025-07-04_17:50:47.786319_bWyP
    • 2025-07-04_17:50:47.786325_ZgLW
    • 2025-07-04_17:50:47.786332_Ravw
    • 2025-07-04_17:50:47.786338_ODeW
    • 2025-07-04_17:50:47.786344_adtW
    f9fdc10
    • 2025-07-07_15:41:50.440045_YTFm
    • 2025-07-07_15:41:50.440081_PHFe
    • 2025-07-07_15:41:50.440093_XrRb
    • 2025-07-07_15:41:50.440104_kHKZ
    • 2025-07-07_15:41:50.440112_oqDE
    • 2025-07-07_15:41:50.440120_AvxA
    • 2025-07-07_15:41:50.440127_aeiG
    • 2025-07-07_15:41:50.440134_Slsj
    • 2025-07-07_15:41:50.440141_IaEq
    • 2025-07-07_15:41:50.440148_WrIl
    redfin-30
    Percentileaad3da8f9fdc10DiffSignificant (?)
    p10218 ±4 ms245 ±6 ms+26.6 ms (+12.2%)MAYBE
    p25225 ±3 ms252 ±5 ms+26.5 ms (+11.8%)YES
    p50232 ±4 ms260 ±6 ms+27.4 ms (+11.8%)MAYBE
    p75240 ±3 ms270 ±7 ms+29.5 ms (+12.3%)MAYBE
    p90248 ±4 ms283 ±7 ms+34.8 ms (+14.0%)YES

    20 test runs in comparison
    CommitTest Runs
    aad3da8
    • 2025-07-04_17:50:47.786239_CMwB
    • 2025-07-04_17:50:47.786285_xldx
    • 2025-07-04_17:50:47.786295_Hhdf
    • 2025-07-04_17:50:47.786304_qdyr
    • 2025-07-04_17:50:47.786312_YotG
    • 2025-07-04_17:50:47.786319_bWyP
    • 2025-07-04_17:50:47.786325_ZgLW
    • 2025-07-04_17:50:47.786332_Ravw
    • 2025-07-04_17:50:47.786338_ODeW
    • 2025-07-04_17:50:47.786344_adtW
    f9fdc10
    • 2025-07-07_15:41:50.440045_YTFm
    • 2025-07-07_15:41:50.440081_PHFe
    • 2025-07-07_15:41:50.440093_XrRb
    • 2025-07-07_15:41:50.440104_kHKZ
    • 2025-07-07_15:41:50.440112_oqDE
    • 2025-07-07_15:41:50.440120_AvxA
    • 2025-07-07_15:41:50.440127_aeiG
    • 2025-07-07_15:41:50.440134_Slsj
    • 2025-07-07_15:41:50.440141_IaEq
    • 2025-07-07_15:41:50.440148_WrIl

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/BHnD79Q8cp/index.html

@dconeybe dconeybe requested a review from Copilot July 3, 2025 18:31
@dconeybe
Copy link
Contributor Author

dconeybe commented Jul 3, 2025

/gemini review

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR replaces the previous UTF-8 byte-oriented string comparison with a more efficient surrogate-aware char-by-char algorithm and updates the changelog to document the fix.

  • Simplified compareUtf8Strings to a single-pass char comparison using Character.isSurrogate
  • Removed ByteString fallback and eliminated codePoint/substring overhead
  • Added an unreleased entry in CHANGELOG.md for the performance fix

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java Rewrote compareUtf8Strings with optimized surrogate-aware logic; removed helper and ByteString paths
firebase-firestore/CHANGELOG.md Documented the UTF-8 comparison performance improvement under Unreleased
Comments suppressed due to low confidence (2)

firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java:90

  • Update this Javadoc to describe the new surrogate-aware, char-by-char comparison algorithm rather than the previous byte-level UTF-8 logic.
  /** Compare strings in UTF-8 encoded byte order */

firebase-firestore/src/main/java/com/google/firebase/firestore/util/Util.java:113

  • Add unit tests covering edge cases around surrogate pairs versus non-surrogate characters to verify the new comparison logic across diverse Unicode inputs.
    for (int i = 0; i < length; i++) {

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a performance improvement to the UTF-8 string comparison logic by avoiding object allocations. Suggestions have been provided to improve code readability.

Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wonderful, thanks Denver!

@dconeybe dconeybe merged commit 84c4d81 into main Jul 7, 2025
34 checks passed
@dconeybe dconeybe deleted the dconeybe/firestore/Utf8StringComparePerformanceFix branch July 7, 2025 16:00
dconeybe added a commit to googleapis/java-firestore that referenced this pull request Jul 7, 2025
The semantics of this logic were originally fixed by #1967, but this fix
caused a material performance degradation, which was then improved by #2021.
The performance was, however, still suboptimal, and this PR further improves the
speed back to close to its original speed and, serendipitously, simplifies the
algorithm too.

This commit effectively ports the following two PRs from the
firebase-android-sdk repository:

- firebase/firebase-android-sdk#7098
- firebase/firebase-android-sdk#7109
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firestore snapshot read performance hit on Android when upgrading to BoM 33.9.0 (Firestore 25.1.2)
4 participants